fix(geometry): validate rotation matrix in Rot3 constructor#2517
fix(geometry): validate rotation matrix in Rot3 constructor#2517jashshah999 wants to merge 2 commits into
Conversation
…t segfault Passing a non-rotation matrix (det != 1 or not orthogonal) to Rot3 caused undefined behavior and segfaults. Add a validity check that throws std::invalid_argument with a clear message. Fixes borglab#2300
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds explicit rotation-matrix validation to Rot3 construction to prevent undefined behavior/segfaults (notably in Python bindings) when given invalid 3×3 matrices.
Changes:
- Validate input matrices in
Rot3matrix constructors and throwstd::invalid_argumenton invalid rotations. - Introduce
Rot3::IsValid(const Matrix3&, double tol)as a public static helper. - Implement orthogonality + determinant checks in
Rot3.cpp.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| gtsam/geometry/Rot3.h | Adds input validation + new IsValid API and documents it. |
| gtsam/geometry/Rot3.cpp | Implements Rot3::IsValid with orthogonality and determinant checks. |
- Fix potential crash in quaternion mode: initialize quaternion_ to identity before validation so garbage input cannot crash Eigen's quaternion-from-matrix before we throw - Extract duplicated error message into private throwIfInvalid() helper - Use 1e-6 tolerance in constructor validation to accept numerically computed rotation matrices (e.g., from matrix exponential) while still rejecting clearly invalid input
|
Please check co-pilot comments. I'll run CI if there is another commit. |
|
Addressed all 4 Copilot comments (replied to each):
All in commit 7c8623b. |
|
Pls check CI issues |
|
It does not make sense to check for validity in the constructor, this adds tons of overhead |
I tend to agree but I'm also a supporter of having more stringent error checking from Python. How could we have our cake and eat it too? In C++ we don't want this overhead. |
|
Good point on overhead. A few options:
Option 3 is probably the best tradeoff — zero overhead in C++, safe from Python. Happy to rework the PR in whichever direction you prefer. |
|
Option 3 seems the best, but I have no idea how to do it :-) |
Summary
Fixes #2300
Passing a non-rotation matrix to
Rot3(e.g., all zeros, non-orthogonal, or det != 1) causes undefined behavior and segfaults in the Python bindings.Added a validity check in the matrix constructor that throws
std::invalid_argumentwhen the input is not a valid rotation matrix (checks orthogonality and determinant with tolerance 1e-9).Also adds a public
Rot3::IsValid(const Matrix3&, double tol)static method that users can call to check validity before constructing.Test plan
RuntimeErrorin Python instead of segfaultingRot3::IsValidcan be used independently to check matrices